LCORE-1329: Adding new MCP E2E Tests#1294
Closed
jrobertboos wants to merge 101 commits intolightspeed-core:mainfrom
Closed
LCORE-1329: Adding new MCP E2E Tests#1294jrobertboos wants to merge 101 commits intolightspeed-core:mainfrom
jrobertboos wants to merge 101 commits intolightspeed-core:mainfrom
Conversation
…ent, and llama-stack-api; add new package 'circuitbreaker' version 2.1.3 and 'oci' version 2.168.0; adjust supported Llama Stack version in constants; fix response handling in utils.
- Introduced new YAML configuration files for MCP authentication methods: file, Kubernetes, and OAuth. - Updated existing MCP configuration to include new authentication methods. - Enhanced end-to-end tests to cover scenarios for file-based, Kubernetes, and OAuth authentication. - Added utility functions for unregistering MCP toolgroups in the testing environment. - Implemented new step definitions for checking response body content in tests.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds end-to-end MCP authentication support and tests: new library/server-mode MCP configs (file, invalid-file, client, kubernetes, oauth), expanded mcp.feature tests, test environment wiring, utilities to unregister MCP toolgroups and clear Llama Stack storage, a secret file, and a docker volume mount for the invalid token. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
- Added a new invalid MCP token file for testing purposes. - Updated the Docker Compose configuration to mount the invalid MCP token. - Introduced a new YAML configuration for testing invalid MCP file authentication. - Enhanced the test scenarios to include checks for invalid MCP file authentication. - Updated feature files to reflect the new authentication configurations.
…d-dependencies LCORE-1326: Updated dependencies
…d-konflux-dependencies LCORE-1326: Updated Konflux dependencies
…moveprefix-method LCORE-1438: Use removeprefix method
…inter-rule LCORE-1437: Enable linter rule B010
…all-commands-during-konflux-requirements-generation LCORE-1439: print all commands during Konflux requirements generation
…ent script Update rag_guide.md to recommend `uv run` instead of `python` for running `llama_stack_configuration.py`, otherwise the dependency on `azure` will not be loaded.
LCORE-1285: Update Llama Stack to 0.5.2
recommend `uv run` instead of `python` in `rag_guide.md`
…-string-format LCORE-1438: better string format
…handling - Added new configurations for invalid, Kubernetes, client, and OAuth MCP authentication methods. - Updated scenario handling to reference the new configuration paths for Kubernetes, client, and OAuth authentication. - Enhanced the testing environment to support the new authentication configurations.
…references/main Update Konflux references
…tograpy-module LCORE-1441: Fixed CVE in cryptography module
…bute LCORE-1402: OKP Index name missing in chunk metadata (tool RAG)lightspeed-core#69
Contributor
|
Summary
🤖 Prompt for AI Agents |
…ze-imports-in-quota-handler LCORE-1438: Organize imports in quota handler
…d-docs LCORE-1438: updated docs
…nagement LCORE-1262: OpenResponses endpoint
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/e2e/features/mcp.feature (1)
10-10:⚠️ Potential issue | 🟠 MajorBlanket
@skiptags still hide core MCP auth regressions.At Line [10], Line [49], Line [65], Line [84], Line [104], Line [146], Line [163], Line [183], and Line [204], unconditional skips disable the exact
/toolsand invalid-token paths this PR is meant to validate. Prefer targeted mode-specific skipping (@skip-in-library-mode) where applicable so non-library runs still exercise these regressions.Also applies to: 49-49, 65-65, 84-84, 104-104, 146-146, 163-163, 183-183, 204-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/mcp.feature` at line 10, Replace the blanket `@skip` tag used across the MCP feature scenarios with targeted mode-specific skips so non-library test runs still execute the important auth paths: locate every occurrence of the '@skip' tag in the MCP feature (the scenarios covering the /tools endpoint and invalid-token flows) and change them to '@skip-in-library-mode' where the test truly should only be skipped in library mode; leave any scenarios that must never run in any mode as explicit skips but prefer the '@skip-in-library-mode' tag for the cases called out (the /tools and invalid-token scenarios) so non-library CI will exercise those regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/features/mcp.feature`:
- Line 10: Replace the blanket `@skip` tag used across the MCP feature scenarios
with targeted mode-specific skips so non-library test runs still execute the
important auth paths: locate every occurrence of the '@skip' tag in the MCP
feature (the scenarios covering the /tools endpoint and invalid-token flows) and
change them to '@skip-in-library-mode' where the test truly should only be
skipped in library mode; leave any scenarios that must never run in any mode as
explicit skips but prefer the '@skip-in-library-mode' tag for the cases called
out (the /tools and invalid-token scenarios) so non-library CI will exercise
those regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fd6b71c-0675-48de-bb0d-2068adb8d55d
📒 Files selected for processing (1)
tests/e2e/features/mcp.feature
- Introduced new YAML configuration files for MCP authentication methods: file, Kubernetes, and OAuth. - Updated existing MCP configuration to include new authentication methods. - Enhanced end-to-end tests to cover scenarios for file-based, Kubernetes, and OAuth authentication. - Added utility functions for unregistering MCP toolgroups in the testing environment. - Implemented new step definitions for checking response body content in tests.
- Added a new invalid MCP token file for testing purposes. - Updated the Docker Compose configuration to mount the invalid MCP token. - Introduced a new YAML configuration for testing invalid MCP file authentication. - Enhanced the test scenarios to include checks for invalid MCP file authentication. - Updated feature files to reflect the new authentication configurations.
…handling - Added new configurations for invalid, Kubernetes, client, and OAuth MCP authentication methods. - Updated scenario handling to reference the new configuration paths for Kubernetes, client, and OAuth authentication. - Enhanced the testing environment to support the new authentication configurations.
- Renamed `llama_stack_shields.py` to `llama_stack_utils.py` and expanded its functionality to manage both toolgroups and shields. - Removed the deprecated `llama_stack_tools.py` file. - Updated E2E test scenarios to utilize the new utility functions for unregistering toolgroups and clearing Llama Stack storage. - Enhanced feature files to include comments indicating pending fixes for skipped scenarios.
- Added checks to ensure the response body does not contain 'mcp-client' in MCP feature tests. - Simplified the `clear_llama_stack_storage` function to remove the entire `~/.llama` directory instead of specific files, improving clarity and efficiency. - Updated comments to reflect the changes in storage clearing logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit
Tests
Chores
Documentation